Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[safe_memcpy_test] Explicit type for arguments of the vector constructor #16549

Merged
merged 2 commits into from
May 19, 2021

Conversation

rialg
Copy link
Contributor

@rialg rialg commented May 18, 2021

Signed-off-by: rialg grial@google.com

Commit Message: Explicit type for arguments of the vector constructor
Additional Description: Issue #16542
Risk Level: N/A

Signed-off-by: rialg <grial@google.com>
@rialg
Copy link
Contributor Author

rialg commented May 18, 2021

Please, @jmarantz and @antoniovicente , could you add yourselves as reviewers?

@jmarantz jmarantz self-assigned this May 18, 2021
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@jmarantz
Copy link
Contributor

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16549 (comment) was created by @jmarantz.

see: more, trace.

@antoniovicente
Copy link
Contributor

Change looks good, but it would be good to hear from @Wyverald on wherever or not this fixes the issue he saw in the bazel build and how to repo that breakage.

@Wyverald
Copy link
Contributor

Thanks for the PR. Unfortunately there isn't an easy way to do a test CI run on a PR (unless we turn the pipeline on for all future Envoy PRs -- which we could certainly do). Is there a way to push this PR to a branch in the main Envoy repo (envoyproxy/envoy)? That would give us a way to test beforehand if this fixes it.

@rialg
Copy link
Contributor Author

rialg commented May 19, 2021

The presubmit pipeline failed due to low code coverage in the tests.

Code coverage for source/common/quic is lower than limit of 88.4 (88.3)

Should we lower this target to match the current value?

@jmarantz
Copy link
Contributor

I think once #16570 merges you should be good? @alyssawilk has another PR also which might just improve the coverage.

@jmarantz
Copy link
Contributor

go ahead an 'merge main' and this coverage issue should be resolved.

Signed-off-by: rialg <grial@google.com>
@antoniovicente
Copy link
Contributor

Thanks for the PR. Unfortunately there isn't an easy way to do a test CI run on a PR (unless we turn the pipeline on for all future Envoy PRs -- which we could certainly do). Is there a way to push this PR to a branch in the main Envoy repo (envoyproxy/envoy)? That would give us a way to test beforehand if this fixes it.

ok, we'll give it another shot if this change isn't sufficient.

@antoniovicente antoniovicente merged commit bf3e6a2 into envoyproxy:main May 19, 2021
@rialg rialg deleted the issue-16542 branch May 20, 2021 14:53
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants